Skip to content
This repository has been archived by the owner on Jun 8, 2021. It is now read-only.

Update RGBA/Rectangle FromValueOptional impls for newly added lifetim… #157

Merged
merged 1 commit into from May 3, 2017
Merged

Update RGBA/Rectangle FromValueOptional impls for newly added lifetim… #157

merged 1 commit into from May 3, 2017

Conversation

sdroege
Copy link
Member

@sdroege sdroege commented May 3, 2017

…e parameter

See gtk-rs/glib#162
As a next step, these two impls can actually benefit from being there for &Rectangle and &RGBA too to prevent some copying. I'll add those later.

@sdroege
Copy link
Member Author

sdroege commented May 3, 2017

What would be the correct way to implement this for e.g. &RGBA? I could just do (with NULL checks, etc):

&*(gobject_ffi::g_value_get_boxed(value.to_glib_none_mut().0) as *const RGBA)

But I would expect that there is some infrastructure for that already? FromGlibPtrBorrow sounds like what I need, but I'm not sure how that is supposed to work.
Same story for Rectangle, which also does not use the normal glib_boxed_wrapper macro, which would implement the above trait.

Should this be the following?

impl<'a> FromGlibPtrBorrow<*mut gdk_ffi::RGBA> for &'a RGBA {
    unsafe fn from_glib_borrow(ptr: *mut gdk_ffi::RGBA) -> Self {
        the_code_from_above
    }
}

@EPashkin
Copy link
Member

EPashkin commented May 3, 2017

@sdroege Sorry I don't understand what you want to do. Don't see any link between GValue and FromGlibPtrBorrow.
Current version won't work?

@sdroege
Copy link
Member Author

sdroege commented May 3, 2017

Current version works, but it will always copy the Rectangle/RGBA. I guess not really important for these cases, but it could also just return a (immutable) reference to them as stored inside the Value.

Question is if there is some infrastructure somewhere to convert a *const gdk_ffi::RGBA to a &gdk::RGBA, or if there should just be the cast as above. FromGlibPtrBorrow sounds like it would do something like that, but I don't fully understand how that's supposed to work.

@EPashkin
Copy link
Member

EPashkin commented May 3, 2017

You mean something like this?

impl<'a> glib::value::FromValueOptional<'a> for &'a RGBA {
    unsafe fn from_value_optional(value: &'a glib::Value) -> Option<Self> {
        let ptr = gobject_ffi::g_value_get_boxed(value.to_glib_none().0) as *mut RGBA;
        if ptr.is_null() { None } else { Some(&*ptr) }
}

It compiles, but not sure that it don't conflict with default implementation and don't cause memory problem

@EPashkin
Copy link
Member

EPashkin commented May 3, 2017

FromGlibPtrBorrow intended to use in signal trampolines for removing unneeded copy or taking ownership. Simply structures like RGBA IMHO always need copy.

@sdroege
Copy link
Member Author

sdroege commented May 3, 2017

They don't need to be copied, but it probably doesn't hurt much to always copy these structs.

The above compiles and works and was exactly what I meant (and is safe to do), but I wasn't sure if there is infrastructure for such casts already. And FromGlibPtrBorrow sounded like it could do exactly that.

@EPashkin
Copy link
Member

EPashkin commented May 3, 2017

impl<'a> FromGlibPtrBorrow<*const gobject_ffi::GValue> for &'a RGBA {
    unsafe fn from_glib_borrow(value: *const gobject_ffi::GValue) -> Self {
        let ptr = gobject_ffi::g_value_get_boxed(value) as *mut RGBA;
        &*ptr
    }
}

Can exist too, but it right variant for Option<&'a RGBA> can't 😭

@EPashkin
Copy link
Member

EPashkin commented May 3, 2017

@GuillaumeGomez restart CI here please

@sdroege
Copy link
Member Author

sdroege commented May 3, 2017

Can exist too, but it right variant for Option<&'a RGBA> can't 😭

Why not? I'll take a look at that tomorrow, but in any case not too important for these types :)

@GuillaumeGomez
Copy link
Member

Restarted.

@EPashkin
Copy link
Member

EPashkin commented May 3, 2017

@GuillaumeGomez CI passed

@GuillaumeGomez
Copy link
Member

Thanks to both of you!

@GuillaumeGomez GuillaumeGomez merged commit 423594c into gtk-rs:master May 3, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants